test: harden clear() namespace isolation, tag invalidation and deleteItem() return value - #124

Nice find on all three. Few things before I merge.

createCachePoolWithOtherNamespace() returns a second FilesystemAdapter, and FilesystemCommonTrait::init() does $directory .= \DIRECTORY_SEPARATOR.$namespace. Two separate directories, so the bug class this is aimed at, prefix collisions on one substrate, structurally can't happen there. Two PdoAdapters over one sqlite connection would bite: doClear() is DELETE ... WHERE id LIKE 'ns%', so the namespace is a real prefix in a shared table. That means moving createCachePool() too though. If you'd rather not, just say in the docblock that it's a smoke test.

Small one: $other->clear() on the last line only runs if every assertion above it passed. try/finally.

On testInvalidateTags, can you keep key tagged ['tag1', 'tag2'] alongside the new key2 and key3? It was the only spot in the suite where a single invalidateTags() call matched one item through two of the passed tags.

Thank you for this review, it is genuinely helpful. You read the actual mechanics of each adapter rather than just the diff, and the FilesystemCommonTrait::init() and PdoAdapter::doClear() details you pointed at are exactly the right ones to weigh here. I appreciate the care.

Points 2 and 3 are fixed.

$other->clear() is now in a finally, so the second pool gets cleaned up even when an assertion above it fails.

On testInvalidateTags, key is tagged ['tag1', 'tag2'] again, so the case where one item is matched through two of the passed tags is back. I kept the discrimination by splitting the rest: key2 carries tag2 alone and key3 carries tag1 alone, so each invalidated tag is now required on its own. key4 carries tag3 and checks that the invalidation does not reach too far.

On the namespace test, I would rather not call it a smoke test, because passing is the expected outcome for a correct implementation. FilesystemAdapter gives each namespace its own directory, so it cannot leak across namespaces, and the test says exactly that. The test is not there to fail on compliant adapters, it is there to fail on the ones that build a shared key space and then clear it with a prefix match that is too loose. That is the class of bug I hit while implementing the PSR-6 MongoDB adapter, where every pool shares one collection.

You are right that the suite here did not exercise a shared substrate, so nothing proved the test would catch a prefix collision. I addressed that on the taggable side, which is where I think it matters most: TaggableCachePoolTest gains the same optional createCachePoolWithOtherNamespace() and a testInvalidateTagsDoesNotAffectOtherNamespaces, and the TaggableCachePool fixture now takes an optional shared storage plus a namespace, so the two pools really do sit on one key space with the namespace as a key prefix. I checked it the other way round too: dropping the prefix from the fixture makes the new test fail, so it does detect the leak.

If you would also like the PSR-6 clear() case covered on a shared substrate, say the word and I will add a test class running the contract over two PdoAdapter instances on one sqlite connection, either here or as a follow up.

The finally and the two-matching-tags case both look right now. And fair on "smoke test", that was a bad word for it.

The shared-storage fixture doesn't do what the docblock says, though. ArrayAdapter implements NamespacedPoolInterface, so ProxyAdapter::__construct takes the withSubNamespace() branch, clones the storage into a fresh store, and sets its own $namespace to ''. No prefix is ever applied.

Probe against the fixture exactly as the test builds it:

shared parent store keys: []
proxy A namespace: ''  proxy B namespace: ''
A inner === storage? false
A inner === B inner? false
A inner keys: ["key"]
B inner keys: ["key"]

Two pools, two separate stores. And dropping the prefix doesn't test the prefix: with '' the branch is skipped entirely, both proxies wrap the same bare ArrayAdapter, and both pools save under the key key. So B's save overwrites A's value before any invalidation runs. That's a key collision, not a tag leak.

Which means the new test can't catch a cross-namespace leak. I made invalidateTags() wipe the whole backing store and it still passed.

The fix is to wrap the storage in the ValidatingCachePool that's already in the fixtures, in both factory methods. It's PSR-6 only, so ProxyAdapter falls to the prefix branch:

return new TaggableCachePool(new Fixtures\ValidatingCachePool($this->sharedStorage()), 'first');
return new TaggableCachePool(new Fixtures\ValidatingCachePool($this->sharedStorage()), 'second');

TaggableCachePool::__construct then needs ?CacheItemPoolInterface instead of ?AdapterInterface. With that, both items land in one ArrayAdapter as firstkey and secondkey, the wipe-everything mutant goes red, and all three CI jobs pass locally: 386 tests / 761 assertions, phpstan clean, cs-check clean.